?

multi should throw an error on attempted executoin of an unstableResp3 command. this enables that.

ok, with what we discussed yesterday, putting the handler on object via attachCommand, this is no longer neccessary, so reverted.

need to update the message + include a link

agreed, but unsure what it should be at the moment.

added a FIXME to it, to know we have to work on that string

move the if + throw to a function and reuse it everywhere?

ok. makes sense. would be a function.

something like

export function resp3UnstableCheck(resp: RespVersions, unstableResp3Command?: boolean, unstableResp3Client?: boolean) {
  if (resp == 3 && unstableResp3Command && !unstableResp3Client) {
    throw new Error("unstable resp3 module, client not configured with proper flag");
  }
}```

and usage ala

  resp3UnstableCheck(resp, command.unstableResp3Module, this._self._self.#options?.unstableResp3Modules);

as discussed on zoom, moved this to how the command is actually attached to object in attachCommand

should be "never" when using RESP3 without enabling unstable commands..

I dont know how to do that into the type (I wasn't eager to extend the typing of the client even further)

I'm not too sure if this is right..

in resp3, the same reply that is possible in resp2 is returned in resp3, just wrapped in an array.

pub fn json_type<M: Manager>(manager: M, ctx: &Context, args: Vec<RedisString>) -> RedisResult {
    let mut args = args.into_iter().skip(1);
    let key = args.next_arg()?;
    let path = Path::new(args.next_str().unwrap_or(default_path()));

    let key = manager.open_key_read(ctx, &key)?;

    let value = if path.is_legacy() {
        json_type_legacy::<M>(&key, path.get_path())?
    } else {
        json_type_impl::<M>(&key, path.get_path())?
    };

    // Check context flags to see if RESP3 is enabled and return the appropriate result
    if is_resp3(ctx) {
        Ok(vec![value].into())
    } else {
        Ok(value)
    }
}

so if one is ok with the resp2, response, the resp3 response looks right to me.

the problem is really in me messing up the conversion in v4, we have

export declare function transformReply(): string | null | Array<string | null>;

so what we need is the ArrayReply<BlobStringReply | NullReply>

made the change I mentioned now

cursorId can go outside the float64 range.. string has to be supported, number is nice to have

ok, was a copy form v4 which used number. trying to remember why I changed it from what you did originally, I suspect number might be used elsewhere and needs to be fixed, will look into that.

so I'm also unconvinced that "number" is nice to have, its a "number", but in practice, cursors should be opaque, and we shouldn't encourage users to think of it as a number, so thinking that RedisArgument as is, should be enough (as you had originally)

so the issue here is really due to AGGREGATE_WITHCURSOR (where cursor id is returned to user). Changed it to be a NumberReply there. I think before I ran into some problems with tests, will have to redo and see what the problem was.

i.e. the problem is that AGGREGATE_WITHCURSOR when cursor is a NumberReply will default to returning as a number.

instead returning NumberReply (from WITHCURSOR) for cursor and taking an UnwrapReply for cursor in cursor read/del and its up to the user to therefore have a proper number conversion setup.

Honestly, not happy with it. I'd want to force the NumberReply to be treated as a string and not as a number in WITHCURSOR (and then can force a string in cursor functions)

"I'd want to force the NumberReply to be treated as a string and not as a number in WITHCURSOR (and then can force a string in cursor functions)" - I'd want the server to reply with a string and not a number..
The server not gonna change, we should probably support both.

so we have the NumberReply that can be both. With that said, I'd think we should document this (i.e. we strongly reccomend one overrides the type mapping for NumberReplies when using the WITHCURSOR function).

A better solution (which I dont know how to implement) would be to be able to tag commands with a default type mapping that can only override default (blank) client type mapping, but not type mapping that the user specifies.

i.e. by default we might say a NumberReply maps to a a js number, but for some commands we can say it maps to string, and the client would then just do the right thing by default always without user intervention required.

same cursor comment

Missing tests

I'm confused. I see a spec.ts

unit tests only..

I am trying to add an execution test, but not sure why I'm seeing output that I am via a decoder

i.e. via telnet

FT.EXPLAIN index '*'
$11
<WILDCARD>

via cli

127.0.0.1:6379> FT.EXPLAIN index '*'
"<WILDCARD>\n"
127.0.0.1:6379>

but via node-redis have to do this to make it pass

assert.equal('<WILDCARD>}\n', reply);

@leibale thoughts?

an object/map in resp2 from the server? no way.. TuplesReply<[timestamp: NumberReply, value: BlobStringReply]>; is right i think

copied from v4 badly I think. will do what you said.

use ArrayReply instead of Array

done and BlobStringReply instead of string and UnwrapReply in transformReply

though had some issues with typing in the transformReply, needs to be validated.

and same in RANGE/MRANGE/_WITHLABELS/

BlobStringReply?

yes, it should be BlobStringReply, this was just a port over from v2, but it's a RedisModule_ReplyWithStringBuffer() so BlobString. Will fix.

addressed above

In RESP3 the reply raw reply type should be

type SampleRawReply3 = ArrayReply<TuplesReply<[timestamp: NumberReply, value: DoubleReply]>>;

(I checked that in TS.RANGE, I guess its true for the rest of them?)

so I'm confused about how tuplereply and array reply work. do they have to be used together?

i.e. in looking at it now i see for range, we basically just have a single element but the function is a an arrayreply wrapping the tuple reply. I'm confused overall.

in looking at it more, I dont see why you would make SampleRawReply3 an ArrayReply, I'd think both resp2/resp3 should just be TupleReply and then RANGE et al would have it wrapped in an ArrayReply as get an array of the tuples. In practice then, resp2/resp3 would be the exact same then (at least for range) and therefore the code could be simplified.

The type in my comments is for samples, but named sample, my bad..

This should work

type SampleRawReply = TuplesReply<[timestamp: NumberReply, value: DoubleReply]>;

const transformSampleReply = {
  2(reply: Resp2Reply<SampleRawReply>) {
    const [ timestamp, value ] = reply as unknown as UnwrapReply<typeof reply>;
    return {
      timestamp,
      value: Number(value)
    };
  },
  3(reply: SampleRawReply) {
    const [ timestamp, value ] = reply as unknown as UnwrapReply<typeof reply>;
    return {
      timestamp,
      value
    };
  }
};

type SamplesRawReply = ArrayReply<SampleRawReply>;

const transformSamplesReply = {
  2(reply: Resp2Reply<SamplesRawReply>) {
    return (reply as unknown as UnwrapReply<typeof reply>)
      .map(sample => transformSampleReply[2](sample));
  },
  3(reply: SamplesRawReply) {
    return (reply as unknown as UnwrapReply<typeof reply>)
      .map(sample => transformSampleReply[3](sample));  }
};

tried implementing this, but cant do it right.

maybe got it now